-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrap transactions as Arc<Transaction>
in TxGraph
#1373
Conversation
b5fdfaa
to
a65b935
Compare
TODO: Make sure all the examples still work. |
TxGraph
.
Main concern: I don't think we need |
a65b935
to
ee66da1
Compare
TxGraph
.Arc<Transaction>
in TxGraph
5e24ad6
to
6786915
Compare
@@ -1006,7 +1006,7 @@ impl<D> Wallet<D> { | |||
/// # let mut wallet: Wallet<()> = todo!(); | |||
/// # let txid:Txid = todo!(); | |||
/// let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx; | |||
/// let (sent, received) = wallet.sent_and_received(tx); | |||
/// let (sent, received) = wallet.sent_and_received(&tx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested to understand why this &
is needed now. Before tx was of type Transaction
right, so shouldn't it needed to have been borrowed before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, TxNode
's tx
field is of a reference to T
. I changed it in this PR to just be tx: T
. Note that T
is now Arc<Transaction>
.
Because sent_and_received
takes in a &Transaction
, we expect to call .as_ref
on Arc<Transaction>
. However, we can make use of the auto-deref properties of AsRef
implementations (such as Arc
), so just doing &tx
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can map to shared ref &
internally and avoid returning Arc<Transaction>
in get_tx{_node}
On second thought, the PR description implies that we specifically DO want to return an arc pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should return a concrete Arc
for this API to be useful (cheaply share transactions between chain-source, receiving-structs and changesets).
Can this PR not be done on top of #1369. I think it can go before and is pretty unrelated. |
cc6c8ad
to
ba5b5d6
Compare
ba5b5d6
to
211a4aa
Compare
Wrapping transactions as `Arc<Transaction>` allows us to share transactions cheaply between the chain-source and receiving structures. Therefore the chain-source can keep already-fetched transactions (save bandwidth) and have a shared pointer to the transactions (save memory). This is better than the current way we do things, which is to refer back to the receiving structures mid-sync. Documentation for `TxGraph` is also updated.
211a4aa
to
8ab58af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8ab58af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8ab58af
May need a rebase.
@@ -430,7 +444,7 @@ impl<A> TxGraph<A> { | |||
/// Note that this only returns directly conflicting txids and won't include: | |||
/// - descendants of conflicting transactions (which are technically also conflicting) | |||
/// - transactions conflicting with the given transaction's ancestors | |||
pub fn direct_conflitcs<'g>( | |||
pub fn direct_conflicts<'g>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏕️ nice cleanup!
96a9aa6 feat(chain): refactor `merge_chains` (志宇) 2f22987 chore(chain): fix comment (志宇) daf588f feat(chain): optimize `merge_chains` (志宇) 77d3595 feat(chain)!: rm `local_chain::Update` (志宇) 1269b06 test(chain): fix incorrect test case (志宇) 72fe65b feat(esplora)!: simplify chain update logic (志宇) eded1a7 feat(chain): introduce `CheckPoint::insert` (志宇) 519cd75 test(esplora): move esplora tests into src files (志宇) a6e613e test(esplora): add `test_finalize_chain_update` (志宇) 494d253 feat(testenv): add `genesis_hash` method (志宇) 886d72e chore(chain)!: rm `missing_heights` and `missing_heights_from` methods (志宇) bd62aa0 feat(esplora)!: remove `EsploraExt::update_local_chain` (志宇) 1e99793 feat(testenv): add `make_checkpoint_tip` (志宇) Pull request description: Fixes #1354 ### Description Built on top of both #1369 and #1373, we simplify the `EsploraExt` API by removing the `update_local_chain` method and having `full_scan` and `sync` update the local chain in the same call. The `full_scan` and `sync` methods now takes in an additional input (`local_tip`) which provides us with the view of the `LocalChain` before the update. These methods now return structs `FullScanUpdate` and `SyncUpdate`. The examples are updated to use this new API. `TxGraph::missing_heights` and `tx_graph::ChangeSet::missing_heights_from` are no longer needed, therefore they are removed. Additionally, we used this opportunity to simplify the logic which updates `LocalChain`. We got rid of the `local_chain::Update` struct (which contained the update `CheckPoint` tip and a `bool` which signaled whether we want to introduce blocks below point of agreement). It turns out we can use something like `CheckPoint::insert` so the chain source can craft an update based on the old tip. This way, we can make better use of `merge_chains`' optimization that compares the `Arc` pointers of the local and update chain (before we were crafting the update chain NOT based on top of the previous local chain). With this, we no longer need the `Update::introduce_older_block` field since the logic will naturally break when we reach a matching `Arc` pointer. ### Notes to the reviewers * Obtaining the `LocalChain`'s update now happens within `EsploraExt::full_scan` and `EsploraExt::sync`. Creating the `LocalChain` update is now split into two methods (`fetch_latest_blocks` and `chain_update`) that are called before and after fetching transactions and anchors. * We need to duplicate code for `bdk_esplora`. One for blocking and one for async. ### Changelog notice * Changed `EsploraExt` API so that sync only requires one round of fetching data. The `local_chain_update` method is removed and the `local_tip` parameter is added to the `full_scan` and `sync` methods. * Removed `TxGraph::missing_heights` and `tx_graph::ChangeSet::missing_heights_from` methods. * Introduced `CheckPoint::insert` which allows convenient checkpoint-insertion. This is intended for use by chain-sources when crafting an update. * Refactored `merge_chains` to also return the resultant `CheckPoint` tip. * Optimized the update `LocalChain` logic - use the update `CheckPoint` as the new `CheckPoint` tip when possible. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: LLFourn: ACK 96a9aa6 Tree-SHA512: 3d4f2eab08a1fe94eb578c594126e99679f72e231680b2edd4bfb018ba1d998ca123b07acb2d19c644d5887fc36b8e42badba91cd09853df421ded04de45bf69
b45897e feat(electrum): update docs and simplify logic of `ElectrumExt` (志宇) 92fb6cb chore(electrum): do not use `anyhow::Result` directly (志宇) b2f3cac feat(electrum): include option for previous `TxOut`s for fee calculation (Wei Chen) c0d7d60 feat(chain)!: use custom return types for `ElectrumExt` methods (志宇) 2945c6b fix(electrum): fixed `sync` functionality (Wei Chen) 9ed33c2 docs(electrum): fixed `full_scan`, `sync`, and crate documentation (Wei Chen) b1f861b feat: update logging of electrum examples (志宇) a6fdfb2 feat(electrum)!: use new sync/full-scan structs for `ElectrumExt` (志宇) 653e4fe feat(wallet): cache txs when constructing full-scan/sync requests (志宇) 58f27b3 feat(chain): introduce `TxCache` to `SyncRequest` and `FullScanRequest` (志宇) 721bb7f fix(chain): Make `Anchor` type in `FullScanResult` generic (志宇) e3cfb84 feat(chain): `TxGraph::insert_tx` reuses `Arc` (志宇) 2ffb656 refactor(electrum): remove `RelevantTxids` and track txs in `TxGraph` (Wei Chen) Pull request description: Fixes #1265 Possibly fixes #1419 ### Context Previous changes such as * Universal structures for full-scan/sync (PR #1413) * Making `CheckPoint` linked list query-able (PR #1369) * Making `Transaction`s cheaply-clonable (PR #1373) has allowed us to simplify the interaction between chain-source and receiving-structures (`bdk_chain`). The motivation is to accomplish something like this ([as mentioned here](#1153 (comment))): ```rust let things_I_am_interested_in = wallet.lock().unwrap().start_sync(); let update = electrum_or_esplora.sync(things_i_am_interested_in)?; wallet.lock().unwrap().apply_update(update)?: ``` ### Description This PR greatly simplifies the API of our Electrum chain-source (`bdk_electrum`) by making use of the aforementioned changes. Instead of referring back to the receiving `TxGraph` mid-sync/scan to determine which full transaction to fetch, we provide the Electrum chain-source already-fetched full transactions to start sync/scan (this is cheap, as transactions are wrapped in `Arc`s since #1373). In addition, an option has been added to include the previous `TxOut` for transactions received from an external wallet for fee calculation. ### Changelog notice * Change `TxGraph::insert_tx` to take in anything that satisfies `Into<Arc<Transaction>>`. This allows us to reuse the `Arc` pointer of what is being inserted. * Add `tx_cache` field to `SyncRequest` and `FullScanRequest`. * Make `Anchor` type in `FullScanResult` generic for more flexibility. * Change `ElectrumExt` methods to take in `SyncRequest`/`FullScanRequest` and return `SyncResult`/`FullScanResult`. Also update electrum examples accordingly. * Add `ElectrumResultExt` trait which allows us to convert the update `TxGraph` of `SyncResult`/`FullScanResult` for `bdk_electrum`. * Added an option for `full_scan` and `sync` to also fetch previous `TxOut`s to allow for fee calculation. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: ValuedMammal: ACK b45897e notmandatory: ACK b45897e Tree-SHA512: 1e274546015e7c7257965b36079ffe0cb3c2c0b7c2e0c322bcf32a06925a0c3e1119da1c8fd5318f1dbd82c2e952f6a07f227a9b023c48f506a62c93045d96d3
Description
This PR makes
TxGraph
store transactions asArc<Transaction>
.Arc<Transaction>
can be shared between the chain-source and receiving structures. This allows the chain-source to keep the already-fetched transactions (save bandwith) and have a shared pointer to the transaction (save memory).Our current logic to avoid re-fetching transactions is to refer back to the receiving structures. However, this means an additional round of locking our receiving structures.
Notes to the reviewers
This will make more sense once I update the esplora/electrum chain sources to make use of both #1369 and this PR. The result would be chain sources which would only require locking the receiving structures twice (once for initiating the update, and once for applying the update).
Changelog notice
TxGraph
to store transactions asArc<Transaction>
. This allows chain-sources to cheaply keep a copy of already-fetched transactions.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features: